Enable importing models created by local users #1037
Conversation
There was a problem hiding this comment.
Hmm.. not really fond of this approach because the user doing the import automatically becomes the model owner.
Here's what i propose:
- whoever is doing the import must be JIMM admin
- we introduce an additional parameter to the command which will specify who is to be set as owner of the model - this way we basically say "we know owner is a local user, but we want that user to map to this specific user"
|
|
@alesstimec @ale8k |
| @@ -40,7 +40,7 @@ jobs: | |||
| - name: Start test environment | |||
| run: docker compose up -d | |||
| - name: Build and Test | |||
| run: go test -mod readonly ./... | |||
| run: go test -mod readonly ./... -coverprofile cover.out | |||
There was a problem hiding this comment.
This is a driveby change to add coverage information to the output of our tests
| @@ -34,7 +34,7 @@ build/server: version/commit.txt version/version.txt | |||
| go build -tags version ./cmd/jimmsrv | |||
|
|
|||
| check: version/commit.txt version/version.txt | |||
| go test -p 1 -timeout 30m -tags version $(PROJECT)/... | |||
| go test -timeout 30m $(PROJECT)/... -coverprofile cover.out | |||
There was a problem hiding this comment.
Also cleaned this up a bit, let me know if you notice anything wrong with these changes. I don't think we are using this make target anywhere though
There was a problem hiding this comment.
Without -p 1 I expect the tests to be run in parallel. Right?
There was a problem hiding this comment.
You need to call t.Parallel to have parallelism in tests
There was a problem hiding this comment.
I believe so, and I assume that's fine because our CI runs the test without that flag set. Though perhaps that's where the failing watcher tests come from? Alex explained above re parallelism
| @@ -34,7 +34,7 @@ build/server: version/commit.txt version/version.txt | |||
| go build -tags version ./cmd/jimmsrv | |||
|
|
|||
| check: version/commit.txt version/version.txt | |||
| go test -p 1 -timeout 30m -tags version $(PROJECT)/... | |||
| go test -timeout 30m $(PROJECT)/... -coverprofile cover.out | |||
There was a problem hiding this comment.
Without -p 1 I expect the tests to be run in parallel. Right?
| owner.SetTag(ownerTag) | ||
| err = j.Database.GetUser(ctx, &owner) | ||
| if ownerTag.IsLocal() { | ||
| return errors.E(op, "cannot import model from local user, try --owner to switch the model owner") |
There was a problem hiding this comment.
This message is a little confusing for me. Also, there was no test to reproduce it.
There was a problem hiding this comment.
Good point, I'll add one
There was a problem hiding this comment.
I think it would be better explained in our documentation, what are local users vs external users. I mentioned --owner to help nudge the user towards reading the jimmctl import-model --help text which explains further
internal/jimm/controller.go
Outdated
| if err != nil { | ||
| return errors.E(op, err) | ||
| errors.E(op, err) |
|
@babakks >Just to make sure things are traceable, does the audit log record contain both the commanding user and the new-owner (if provided with --owner option)? The audit logs should contains all the params for the call so it would include the user and the new owner |
Description
Add the ability for a JIMM admin to import a model that belongs to a local user of a controller.
Currently importing a model that already exists but belongs to a local user will fail because JIMM requires that all models have an associated cloud-credential. A cloud-credential for a local user will never be something that JIMM is aware of.
A good follow-up to this would be the removal of associating cloud-credentials with a model since a cloud-credential is only needed once, when the model is created so it doesn't make sense to persist that info in JIMM.
Fixes CSS-4612
Engineering checklist
Check only items that apply